Skip to content

feat(mcp): OAuth 2.1 + PKCE for outbound MCP servers#4441

Merged
waleedlatif1 merged 18 commits into
stagingfrom
waleedlatif1/mcp-oauth
May 20, 2026
Merged

feat(mcp): OAuth 2.1 + PKCE for outbound MCP servers#4441
waleedlatif1 merged 18 commits into
stagingfrom
waleedlatif1/mcp-oauth

Conversation

@waleedlatif1
Copy link
Copy Markdown
Collaborator

Summary

  • Adds spec-compliant OAuth 2.1 + PKCE support for outbound MCP servers via the SDK's OAuthClientProvider
  • Auto-detects OAuth requirement on server create via unauthenticated probe (WWW-Authenticate / oauth-protected-resource)
  • Persists per-user-per-server tokens (encrypted) in new mcp_server_oauth table; SDK refreshes automatically before expiry
  • Popup-based consent flow (/api/mcp/oauth/start/api/mcp/oauth/callback) with state CSRF protection
  • Pre-registered OAuth client support (Client ID + Secret in Advanced settings) for servers without RFC 7591 DCR
  • Surfaces reauth_required from tool execution when refresh token is invalid so the UI can prompt to reconnect

Type of Change

  • New feature

Testing

Tested manually against OAuth-protected MCP servers (Linear). Existing header-auth servers regression-checked.

Checklist

  • Code follows project style guidelines
  • Self-reviewed my changes
  • Tests added/updated and passing
  • No new warnings introduced
  • I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

@vercel
Copy link
Copy Markdown

vercel Bot commented May 5, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
docs Ready Ready Preview, Comment May 20, 2026 6:47pm

Request Review

@cursor
Copy link
Copy Markdown

cursor Bot commented May 5, 2026

PR Summary

High Risk
High risk because it introduces new OAuth authorization endpoints and changes MCP server/tool execution error handling, including storage/handling of OAuth client secrets and new 401 reauth paths.

Overview
Adds OAuth support for outbound MCP servers. Introduces /api/mcp/oauth/start and /api/mcp/oauth/callback to run an OAuth (PKCE) popup flow, validate state/user/workspace, enforce safe server URLs, and refresh tool discovery after authorization.

Expands MCP server create/update/list APIs and UI. Server CRUD now accepts authType plus optional oauthClientId/oauthClientSecret, returns hasOauthClientSecret instead of the raw secret, and the settings/workflow modals add an Advanced section to manage these fields and trigger OAuth connect.

Improves MCP tool discovery/execution failure handling. Discovery and execution routes treat OAuth-required/unauthorized errors as 401 “reauth required” (with serverId for execution) and the execution route tightens schema coercion and clears timeout timers.

Docs-only additions. Adds several .agents/skills/source-command-* markdown skill templates for adding/validating connectors, tools, and triggers.

Reviewed by Cursor Bugbot for commit b7c937d. Configure here.

Comment thread apps/sim/lib/mcp/oauth/provider.ts
Comment thread apps/sim/hooks/queries/mcp.ts
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 5, 2026

Greptile Summary

This PR adds spec-compliant OAuth 2.1 + PKCE support for outbound MCP servers, including auto-detection via an unauthenticated probe, a popup-based consent flow, encrypted per-server token storage, and pre-registered client credential support.

  • New mcp_server_oauth table and storage layer — holds encrypted tokens, PKCE verifier, and OAuth client info per server; a dedicated stateCreatedAt column decouples state TTL from token-refresh writes to prevent 409 loops.
  • OAuth provider and SDK integrationSimMcpOauthProvider implements the SDK's OAuthClientProvider; service.ts wraps connections with a per-row refresh lock and handles UnauthorizedError/McpOauthAuthorizationRequiredError by marking servers disconnected rather than error.
  • UI flow — a new useMcpOauthPopup hook manages per-server connecting state, popup lifecycle, and postMessage callbacks; the form modal adds Advanced settings for pre-registered client credentials with proper secretTouched tracking.

Confidence Score: 3/5

The delete path can revoke another workspace's OAuth tokens before confirming ownership; safe to merge only after moving revocation to after the delete confirms the row belongs to the caller.

The performDeleteMcpServer function calls revokeMcpOauthTokens(params.serverId) before the db.delete that enforces workspace scope. The fix was applied to the old route handler but was not carried forward when the logic was extracted into this shared lifecycle function, so any authenticated user can force another workspace's MCP server into a re-auth state by sending a DELETE with a foreign serverId. The rest of the OAuth implementation — PKCE flow, encrypted storage, state TTL anchoring, per-row refresh lock, and UI consent flow — is well-structured and addresses the issues raised in earlier review rounds.

apps/sim/lib/mcp/orchestration/server-lifecycle.ts — the performDeleteMcpServer function needs the revocation call moved to after ownership is confirmed.

Security Review

  • Cross-workspace token revocation (apps/sim/lib/mcp/orchestration/server-lifecycle.ts): revokeMcpOauthTokens in performDeleteMcpServer fires before the db.delete workspace ownership check, allowing an authenticated user to revoke another workspace's OAuth tokens by supplying an arbitrary serverId.
  • All other previously flagged security issues (CSRF state validation, javascript: URL blocking, early-return 404 before revocation in PATCH, workspace-scoped server lookup in DELETE route) appear to have been addressed in the current diff.

Important Files Changed

Filename Overview
apps/sim/lib/mcp/orchestration/server-lifecycle.ts Core lifecycle orchestration for MCP server CRUD; performDeleteMcpServer still calls token revocation before workspace-scoped ownership is confirmed, enabling cross-workspace revocation.
apps/sim/app/api/mcp/oauth/callback/route.ts New OAuth callback handler; properly burns state before token exchange, validates user session, checks workspace ownership, and escapes HTML in popup messages.
apps/sim/app/api/mcp/oauth/start/route.ts New OAuth start handler; validates server ownership, URL safety, and active-flow TTL before initiating the PKCE flow via the SDK provider.
apps/sim/lib/mcp/oauth/storage.ts OAuth persistence layer; encrypts tokens, client information, and code verifier; anchors state TTL on dedicated stateCreatedAt column so token refreshes don't extend the active-flow window.
apps/sim/lib/mcp/oauth/provider.ts SDK-compatible OAuthClientProvider implementation; delegates storage to the encrypted DB layer and supports pre-registered client credentials to bypass DCR.
apps/sim/lib/mcp/service.ts Adds OAuth-aware createClient path with per-row refresh lock; handles UnauthorizedError and McpOauthAuthorizationRequiredError by marking servers disconnected rather than error.
packages/db/schema.ts Adds authType, oauthClientId, oauthClientSecret to mcpServers and introduces the new mcp_server_oauth table with proper cascade deletes and a dedicated stateCreatedAt column.
apps/sim/hooks/queries/mcp.ts Refactors query keys to a two-level hierarchy, converts useForceRefreshMcpTools to a proper useMutation, and adds useStartMcpOauth with HTTPS scheme validation before opening the popup.
apps/sim/app/api/mcp/tools/execute/route.ts Adds OAuth re-auth error handling returning a typed reauth_required 401; fixes timeout handle leak with .finally(clearTimeout) and removes the any cast on input schema properties.

Comments Outside Diff (1)

  1. apps/sim/lib/mcp/orchestration/server-lifecycle.ts, line 416-424 (link)

    P1 security Cross-workspace revocation before ownership check

    revokeMcpOauthTokens(params.serverId) fires before db.delete proves the server belongs to the caller's workspace. An authenticated user in workspace A can send DELETE /api/mcp/servers/<id-from-workspace-B>; revocation executes against the external authorization server and invalidates workspace B's tokens, while the subsequent db.delete silently returns zero rows (wrong workspaceId) and returns not_found. The previous fix was applied to the old route handler, but was not carried forward when this logic was extracted into performDeleteMcpServer.

    Move revokeMcpOauthTokens to after server is confirmed non-null (i.e., after if (!server) return ...), mirroring how performUpdateMcpServer first fetches currentServer with a workspace filter before calling revocation.

Reviews (42): Last reviewed commit: "fix(mcp): normalize empty-string oauthCl..." | Re-trigger Greptile

Comment thread apps/sim/app/workspace/[workspaceId]/settings/components/mcp/mcp.tsx Outdated
Comment thread apps/sim/lib/mcp/oauth/storage.ts
Comment thread apps/sim/lib/mcp/oauth/storage.ts Outdated
@waleedlatif1
Copy link
Copy Markdown
Collaborator Author

Greptile summary findings addressed in f587e82:

  • Edit modal drops existing OAuth Client ID: editInitialData now includes oauthClientId; the API already returns it (only the secret is masked) so the field populates and Advanced auto-expands.
  • Shared OAuth mutation disables all buttons: per-server pending tracked in a local Set<string>; the spinner is scoped to the card whose flow is in progress.
  • Plaintext PKCE codeVerifier: now encrypted at rest via encryptSecret to match tokens/clientInformation.

The point about clearing a pre-registered Client ID by emptying the field is a follow-up — oauthClientId || undefined collapses an intentional clear into a no-op. Will tackle when adding TTL cleanup for abandoned OAuth sessions.

@waleedlatif1
Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1
Copy link
Copy Markdown
Collaborator Author

@cursor review

Comment thread apps/sim/app/api/mcp/servers/[id]/route.ts Outdated
Comment thread apps/sim/app/api/mcp/servers/[id]/route.ts Outdated
@waleedlatif1
Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1
Copy link
Copy Markdown
Collaborator Author

@cursor review

Comment thread apps/sim/app/api/mcp/servers/[id]/route.ts Outdated
@waleedlatif1
Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1
Copy link
Copy Markdown
Collaborator Author

@cursor review

Comment thread apps/sim/app/api/mcp/servers/route.ts Outdated
@waleedlatif1
Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1
Copy link
Copy Markdown
Collaborator Author

@cursor review

Comment thread apps/sim/app/api/mcp/tools/execute/route.ts
Comment thread apps/sim/lib/mcp/service.ts Outdated
Comment thread apps/sim/lib/mcp/service.ts Outdated
@waleedlatif1
Copy link
Copy Markdown
Collaborator Author

@cursor review

@waleedlatif1
Copy link
Copy Markdown
Collaborator Author

@greptile

Comment thread apps/sim/app/api/mcp/oauth/callback/route.ts
Comment thread apps/sim/app/api/mcp/servers/route.ts
The POST /api/mcp/servers handler omitted authType from the success
response, so useCreateMcpServer always saw data.data.authType as
undefined and never triggered the OAuth popup after creating an
OAuth-protected server. Thread authType through performCreateMcpServer
into the response so the client can decide whether to auto-start OAuth.
@waleedlatif1
Copy link
Copy Markdown
Collaborator Author

@cursor review

Comment thread apps/sim/lib/mcp/orchestration/server-lifecycle.ts
Comment thread apps/sim/lib/mcp/orchestration/server-lifecycle.ts
Comment thread apps/sim/hooks/queries/mcp.ts
…d update

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@waleedlatif1
Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1
Copy link
Copy Markdown
Collaborator Author

@cursor review

…rver type

The response contract preprocesses null → undefined, so McpServer.oauthClientId
is string | undefined. Using null broke type checking.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Comment thread apps/sim/lib/mcp/orchestration/server-lifecycle.ts
@waleedlatif1
Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1
Copy link
Copy Markdown
Collaborator Author

@cursor review

Comment thread apps/sim/hooks/mcp/use-mcp-oauth-popup.ts
Comment thread apps/sim/lib/mcp/oauth/probe.ts
- probe: only classify as OAuth on resource_metadata or scope params.
  Bare `Bearer error="invalid_token"` is generic and used by API-key servers,
  so it must not auto-flip the auth type to OAuth.
- popup hook: clear any existing close-watcher interval before overwriting
  when startOauthForServer is invoked twice for the same serverId.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@waleedlatif1
Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1
Copy link
Copy Markdown
Collaborator Author

@cursor review

Comment thread apps/sim/app/api/mcp/servers/[id]/route.ts Outdated
Orchestration already converts falsy → null via `|| null` (server-lifecycle.ts),
so the DB was never receiving an empty string. Tightening the route layer to
match the same convention keeps the boundary contract consistent and avoids
relying on downstream normalization.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@waleedlatif1
Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1
Copy link
Copy Markdown
Collaborator Author

@cursor review

Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit b7c937d. Configure here.

waleedlatif1 and others added 9 commits May 19, 2026 19:12
The MCP Tool block on the workflow canvas previously crammed every selected-
tool parameter into a stringified blob under the `Tool` row. Now, when a tool
is selected, the tile reads the cached `_toolSchema` and emits one labeled
SubBlockRow per parameter (matching the Exa block's per-param layout). Labels
reuse `formatParameterLabel` for parity with the editor panel; values pass
through the existing `getDisplayValue` so booleans/numbers/arrays render
identically to other blocks. Deterministic tile height counts expanded rows
so the tile sizes correctly.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Tool spans for MCP calls were rendering the raw id (e.g.
`mcp-f908f259-planetscale_list_organizations`) with the default blank-
square icon. Now they read just the tool name and render the MCP block's
icon and bgColor, matching how workflow-execute tools render.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Block bgColors below a small luminance threshold (e.g. the MCP block's
#181C1E) rendered nearly invisible against the dark-mode surface
(--bg: #1b1b1b). Adds a tiny adjustBgForContrast helper that floors each
RGB channel at 0x33 only when luminance is below 30,000, leaving every
branded color above that band untouched. Applied to both the trace tree
row and the detail pane.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
#333333 was still too close to the dark-mode surface to read. For bgs
below the luminance threshold (e.g. the MCP block's #181C1E) we now fall
back to DEFAULT_BLOCK_COLOR (#6b7280) — the same neutral the renderer
uses for blocks with no distinct identity. Clearly visible in both
themes; brighter brand colors still pass through.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Staging shipped 0209_smiling_fixer; the MCP OAuth migration will be
regenerated on top of staging as 0210.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…auth

# Conflicts:
#	scripts/check-api-validation-contracts.ts
Re-runs drizzle-kit generate on top of staging's 0209_smiling_fixer.
Same schema (mcp_server_oauth table + mcp_servers.auth_type / oauth_*
columns) as the dropped 0209_mcp_oauth.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The post-merge route count is 749 (this branch's OAuth start/callback
plus staging's new route). I had set the baseline to 748 in the merge
conflict resolution — bumping to match reality so the strict audit
passes.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
These were untracked-then-accidentally-staged in 05c4bc1 via a wide
`git add -A`. They aren't part of this PR's scope.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant